Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove pulse support in QPY in 2.0 #13814

Merged
merged 20 commits into from
Feb 26, 2025
Merged

Remove pulse support in QPY in 2.0 #13814

merged 20 commits into from
Feb 26, 2025

Conversation

eliarbel
Copy link
Contributor

@eliarbel eliarbel commented Feb 9, 2025 •
edited
Loading

Summary

This PR removes support for saving ScheduleBlock programs in qpy.dump. It also includes support for loading payloads with ScheduleBlocks or pulse gates while ignoring the actual pulse information, returning partially specified circuits.

Part of #13662

Details and comments

Some details to note and give feedback on:

  1. Loading a ScheduleBlock program via qpy.load results with a QuantumCircuit object containing only the name and possibly metadata of that ScheduleBlock. Loading a QuantumCircuit payload with pulse gates leads to getting circuit object with undefined custom instructions (representing pulse gates without calibrations). In both cases warnings of type QPYLoadingDeprecatedFeatureWarning are being issued. Is this the type of warning we want to use here? Maybe we should add something like QPYLoadingRemovedFeatureWarning and update the Compatibility section in the QPY doc page accordingly?
  2. This PR does not change the QPY format version, i.e. it stays on 13. This means that we still write CALIBRATION header (with 0 cals) so that load continues to work (since it still includes code for reading calibrations, for graceful backward compatibility).
  3. When loading payloads with calibrations, the code will issue a message for each calibration, warning the user that if there exists a gate with this name, it will be undefined. Theoretically, one can add calibrations to a circuit without any pulse gates. We could check this and have the warnings accurately relate to existing pulse gates, but it think it'll just impose extra runtime overhead. I'm open to other opinions though.

Open Tasks:

  • Complete handling of test/qpy_compat

@eliarbel eliarbel added Changelog: Removal Include in the Removed section of the changelog mod: qpy Related to QPY serialization labels Feb 9, 2025
@eliarbel eliarbel added this to the 2.0.0 milestone Feb 9, 2025
@coveralls
Copy link

coveralls commented Feb 9, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 13522069571

Details

  • 19 of 61 (31.15%) changed or added relevant lines in 4 files are covered.
  • 96 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.1%) to 87.831%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qpy/interface.py 5 7 71.43%
qiskit/qpy/binary_io/circuits.py 3 10 30.0%
qiskit/qpy/binary_io/schedules.py 9 42 21.43%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.23%
qiskit/pulse/builder.py 1 85.31%
qiskit/pulse/instructions/reference.py 1 90.32%
qiskit/qpy/binary_io/circuits.py 2 88.79%
qiskit/qpy/interface.py 2 86.15%
qiskit/qpy/type_keys.py 4 82.16%
crates/qasm2/src/lex.rs 5 92.48%
qiskit/qpy/binary_io/value.py 5 86.26%
qiskit/pulse/library/symbolic_pulses.py 8 93.01%
crates/qasm2/src/parse.rs 18 96.68%
Totals Coverage Status
Change from base Build 13512719904: -0.1%
Covered Lines: 77084
Relevant Lines: 87764
💛 - Coveralls

@1ucian0 1ucian0 changed the title Remove pulse support in QPY 2.0 Remove pulse support in QPY in 2.0 Feb 10, 2025
@eliarbel eliarbel marked this pull request as ready for review February 11, 2025 12:19
@eliarbel eliarbel requested a review from a team as a code owner February 11, 2025 12:19
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

raynelfss
raynelfss reviewed Feb 11, 2025
View reviewed changes
Copy link
Contributor

@raynelfss raynelfss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a typo I found :)

eliarbel and others added 2 commits February 12, 2025 10:48
@eliarbel @raynelfss
Update qiskit/qpy/binary_io/circuits.py
9ea2fd3 
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
@eliarbel
Merge branch 'main' into remove-pulse-qpy
7aff7e2
@eliarbel eliarbel marked this pull request as draft February 13, 2025 17:32
@eliarbel eliarbel marked this pull request as ready for review February 16, 2025 13:43
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

@eliarbel eliarbel mentioned this pull request Feb 17, 2025
Merged
@eliarbel eliarbel added the mod: pulse Related to the Pulse module label Feb 17, 2025
@mtreinish mtreinish self-assigned this Feb 18, 2025
@eliarbel eliarbel marked this pull request as draft February 18, 2025 15:12
@eliarbel eliarbel marked this pull request as ready for review February 19, 2025 14:34
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core
  • @mtreinish
  • @nkanazawa1989

mtreinish
mtreinish reviewed Feb 19, 2025
View reviewed changes
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. I left a few comments and suggestions inline, but nothing major.

I was confused at first by the changes in the binary_io/schedules.py flle because I skipped over the module docs. But it's clear now that it's basically a bunch of seek() methods now. It's obvious now they're needed because the size to read for the pulse parts isn't known ahead of time, so this is needed to skip the pulse portion of a pulse gates circuit payload.

@eliarbel
Copy link
Contributor Author

Thanks for the review @mtreinish, all comments have been addressed.

mtreinish
mtreinish approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, I had two small inline comments but nothing worth blocking over. Mostly my idle musings and one extra space in a docstring. Thanks for doing this and the quick updates.

@mtreinish mtreinish added this pull request to the merge queue Feb 25, 2025
Merged via the queue into Qiskit:main with commit f1729db Feb 26, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raynelfss raynelfss raynelfss left review comments

@mtreinish mtreinish mtreinish approved these changes

Assignees

@mtreinish mtreinish

@eliarbel eliarbel

Labels
Changelog: Removal Include in the Removed section of the changelog mod: pulse Related to the Pulse module mod: qpy Related to QPY serialization
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@eliarbel @coveralls @qiskit-bot @mtreinish @raynelfss